design-proposals: tenant module overrides#4
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a design proposal for per-tenant module overrides: typed Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / cozyvalues-gen
participant CI as Build/CI
participant Helm as Helm templates
participant K8s as Kubernetes (Secrets, HelmRelease)
participant Cluster as Cluster controllers
Dev->>CI: commit chart with `@embed <package> as Alias` annotations
CI->>Dev: run cozyvalues-gen -> generate typedefs & schema
CI->>Helm: package charts with embedded override schemas
Helm->>K8s: render tenant HelmRelease with module object normalization
Helm->>K8s: create per-module override Secret when `enabled: true`
K8s->>Cluster: attach Secret to HelmRelease.valuesFrom (after `cozystack-values`)
Cluster->>K8s: reconcile HelmRelease, apply tenant module values
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This design proposal introduces per-tenant overrides for bundled modules such as etcd and seaweedfs by transitioning the module configuration from a boolean to an object structure. It includes a new @embed directive for the cozyvalues-gen tool to automate schema generation and updates Helm templates to merge tenant-specific values via Flux's valuesFrom mechanism. The review feedback correctly identifies a documentation inconsistency regarding the JSON schema path for valuesOverride and points out necessary syntax corrections for invoking the dict function within the Go template examples to ensure they function as intended.
|
|
||
| Generator output side effects: | ||
|
|
||
| - Tenant chart's `values.schema.json` gains the full etcd sub-schema under `properties.etcd.properties.values`. |
There was a problem hiding this comment.
The JSON schema path mentioned here (properties.etcd.properties.values) does not match the field name valuesOverride defined in the design (lines 71, 76, 82). It should be updated for consistency.
| - Tenant chart's `values.schema.json` gains the full etcd sub-schema under `properties.etcd.properties.values`. | |
| - Tenant chart's values.schema.json gains the full etcd sub-schema under properties.etcd.properties.valuesOverride. |
There was a problem hiding this comment.
Fixed — renamed to properties.etcd.properties.valuesOverride to match the field name used in Design §1 and the rest of the proposal. Commit: 8aae9cd.
| {{- $etcd := .Values.etcd }} | ||
| {{- if kindIs "bool" $etcd }} | ||
| {{- /* Bool-form compatibility shim. Normalized to object-form. */ -}} | ||
| {{- $etcd = dict "enabled" $etcd "valuesOverride" dict }} |
There was a problem hiding this comment.
In Helm/Sprig templates, dict is a function. To assign an empty dictionary as a value, you must invoke it using parentheses: (dict). Without parentheses, you are passing the function reference itself as the value for the valuesOverride key.
| {{- $etcd = dict "enabled" $etcd "valuesOverride" dict }} | |
| {{- $etcd = dict "enabled" $etcd "valuesOverride" (dict) }} |
There was a problem hiding this comment.
Fixed — switched to (dict). Go templates do resolve dict as a zero-arg call in argument position, so this was a cosmetic fix, but the parens make the intent unambiguous and match the idiom used across the Cozystack Helm charts. Commit: 8aae9cd.
| type: Opaque | ||
| stringData: | ||
| values.yaml: | | ||
| {{ toYaml (default dict $etcd.valuesOverride) | indent 4 }} |
There was a problem hiding this comment.
The dict function should be invoked as (dict) when used as an argument to the default function. This ensures that an empty map is provided as the default value if valuesOverride is empty or nil.
| {{ toYaml (default dict $etcd.valuesOverride) | indent 4 }} | |
| {{ toYaml (default (dict) $etcd.valuesOverride) | indent 4 }} |
There was a problem hiding this comment.
Fixed — same treatment as the shim a few lines above: default (dict) $etcd.valuesOverride. Commit: 8aae9cd.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design-proposals/tenant-module-overrides/README.md`:
- Around line 109-110: The docs/spec mix two field names for the same concept
(etcd.valuesOverride vs etcd.values) causing schema and README drift; pick and
use one name consistently across the proposal and generated artifacts. Update
the proposal text, the example lines that mention `values` (e.g., the sentence
about Tenant chart's `values.schema.json` and the generated README reference
lines around 109 and 211) to use the canonical `valuesOverride` identifier, and
verify any schema property paths (`properties.etcd.properties.values`) are
renamed to `properties.etcd.properties.valuesOverride` so the generated README
and schema match.
- Around line 180-185: The documentation and migration scope are inconsistent:
either restrict the migration to only rewrite etcd boolean values or explicitly
state that Release N templates already normalize bool/object-form for
monitoring, ingress, and seaweedfs before this migration runs. Update the README
so the migration description and the “Release N” rollout notes match: if keeping
the multi-module rewrite (etcd, monitoring, ingress, seaweedfs) ensure you add a
note that the cozystack-tenant chart templates normalize bool/object-form for
monitoring/ingress/seaweedfs prior to migration; otherwise change the migration
steps to only list/modify HelmRelease spec.values.etcd and leave
monitoring/ingress/seaweedfs untouched until their templates/schema are safe to
accept object-form. Ensure references to HelmRelease, cozystack-tenant, etcd,
monitoring, ingress, seaweedfs, and “Release N” are updated consistently.
- Around line 90-92: The fenced code block containing "@embed <package-path> as
<TypedefName>" is missing a fence language and triggers MD040; update that block
(the triple-backtick block that wraps "@embed <package-path> as <TypedefName>")
to include a language like "text" so it becomes "```text" at the opening fence,
preserving the content and closing fence unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ab29c9e-0587-4034-b66a-e0e88f476bed
📒 Files selected for processing (1)
design-proposals/tenant-module-overrides/README.md
Proposes replacing the Tenant chart's boolean module flags (etcd/monitoring/ingress/seaweedfs) with typed override objects, sourced through a new @embed directive in cozyvalues-gen. Piloted on etcd; monitoring/ingress/seaweedfs follow the same primitive in later PRs. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
myasnikovdaniil
force-pushed
the
proposal/tenant-module-overrides
branch
from
68f63a9 to
0319581
Compare
- Fix schema path in Design §2 to match the valuesOverride field name. - Invoke dict() explicitly as (dict) in the Helm template schematics for clarity. - Add a fence language to the @embed syntax block. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
2 participants
Summary
Proposes a design for per-tenant overrides of bundled Tenant modules
(etcd/monitoring/ingress/seaweedfs). Currently each module is a boolean
flag and its values come only from the cluster-wide
cozystack-valuesSecret, leaving admins with no override path short of disabling the
module and hand-rolling a HelmRelease (which drops tenant scaffolding,
ApplicationDefinition wiring, labels, and quotas).
The proposal pilots the mechanism on
etcd:etcd: boolbecomesetcd: { enabled, valuesOverride }.@embeddirective in cozyvalues-gen. Pulls the etcd chart'svalues.yamlannotations into the tenant chart's schema at buildtime. Schema truth stays in one place; no hand-duplicated types.
valuesFrom→ tenant deep-merges on top of cluster defaults.object-form; the Helm template also keeps a bool-form compatibility
shim for two releases. Zero breakage for existing Tenant manifests.
follow-up release with no new design work.
Scope is deliberately narrow: multi-etcd-per-tenant and the broader
admin customization/overlay story are called out as sibling proposals.
Test plan
@embeddirective fits the cozyvalues-gen roadmapSummary by CodeRabbit